[TRTLLMINF-191][infra] Use native pytest capture for S3 logs by niukuo · Pull Request #16739 · NVIDIA/TensorRT-LLM

niukuo · 2026-07-22T14:05:50Z

Dev Engineer Review

Migrated S3 log handling from legacy stdout/session + --s3-echo-stdout/ENABLE_S3_ECHO_STDOUT wiring to native pytest --capture=fd with report-section transformation.
Standardized pytest extra args when ENABLE_UPLOAD_TEST_RESULTS is enabled (both SLURM sbatch and non-SLURM execution paths):
- --capture=fd
- --s3-upload-path=${uploadPath}/${stageName}
- --s3-upload-mode=deferred
- Removed prior -s usage and legacy/conditional S3 argument wiring (including --s3-echo-stdout and prior timestamped/session capture mode logic).
Reworked deferred upload implementation in tests/test_common/s3_output.py:
- Spool-backed capture transformation with inline-vs-upload thresholding.
- sync vs deferred upload modes with bounded concurrency (ThreadPoolExecutor).
- Deferred mode deletes spool sources only after successful upload; upload failures preserve relevant spool/report details.
Added drain_pending_uploads(output_path, secret_key) to recover orphaned .s3-spool artifacts from crashed pytest processes using per-owner coordination (fcntl.flock + hostname/pid liveness checks).
Simplified pytest hook registration in tests/test_common/s3_output_hooks.py:
- Removed early-conftest capture/claimed-state machinery.
- Transformer plugin registered only on xdist workers (skip controller).
Added/adjusted lifecycle integration:
- jenkins/scripts/slurm_run.sh drains spool after eval $pytestCommand completes (failures tolerated so existing diagnostics/exit handling can run).
- tests/integration/defs/test_unittests.py conditionally enables S3 handling when --s3-upload-path is set and drains pending uploads in a finally block.
API consistency changes to verify across call sites:
- UploadLogPlugin constructor signature change (removal of older echo/session capture-related parameters).
- register_plugin(config) signature change and enforced requirement of native --capture=fd when S3 upload is enabled.

Potential review focus:

Correctness of spool ownership/drain selection under pid reuse and concurrent drains (flock scope + liveness checks).
Deferred upload lifecycle ordering and cleanup guarantees on partial failures/timeouts.
Ensuring every S3-enabled command line path enforces --capture=fd (and that any external invocations of register_plugin/plugin constructor are updated accordingly).

QA Engineer Review

Test code changes include (tests/ touched):

`tests/unittest/test_s3_output.py` (rewritten/expanded behavioral coverage)

Modified/added test functions:

test_small_stdout_remains_inline
test_stdout_at_threshold_is_replaced_with_url
test_inline_threshold_applies_to_combined_stream
test_logging_section_is_uploaded_even_when_small
test_sync_upload_transforms_native_sections
test_duplicate_capture_sections_share_one_object
test_cumulative_capture_sections_are_uploaded_once
test_same_nodeid_rerun_gets_distinct_test_path
test_failed_report_keeps_only_recent_bounded_output
test_deferred_upload_starts_before_session_finish
test_deferred_upload_reuses_cumulative_section
test_rerun_keeps_one_url_per_attempt
test_parent_drain_retries_upload_left_by_failed_process
test_parent_drain_uses_configured_upload_workers
test_parent_drain_skips_live_pytest_process
test_register_plugin_requires_native_fd_capture
test_register_plugin_uses_report_transformer
test_s3_hook_skips_xdist_controller
test_native_capture_is_replaced_before_junit_consumes_report
test_rerun_urls_are_distinct_and_line_delimited_in_junit
test_xdist_worker_transforms_report_before_controller_junit
test_native_capture_restores_timeout_output_to_console

`tests/unittest/tools/test_test_to_stage_mapping.py`

Modified/added:
- test_s3_output_uses_native_fd_capture (replaces the removed --s3-echo-stdout/ENABLE_S3_ECHO_STDOUT opt-in validation)

`tests/integration/defs/test_unittests.py`

Modified unittest runner logic via:
- merge_report
- test_unittests_v2

Coverage in tests/integration/test_lists/ / test-db/ / qa/:

Not determined from the provided context (no test-list-only changes were indicated). Verdict: needs follow-up.

Description

Test Coverage

PR Checklist

Please review the following before submitting your PR:

PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either api-compatible or api-breaking. For api-breaking, include BREAKING in the PR title.
Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.

GitHub Bot Help

To see a list of available CI bot commands, please comment /bot help.

niukuo · 2026-07-22T14:06:09Z

/bot run --disable-fail-fast

coderabbitai · 2026-07-22T14:13:30Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 7cdb9a5c-9644-49ff-9180-d04dc4ef97b7

📥 Commits

Reviewing files that changed from the base of the PR and between ba8cca9 and e764c3c.

📒 Files selected for processing (2)

tests/test_common/s3_output.py
tests/unittest/test_s3_output.py

🚧 Files skipped from review as they are similar to previous changes (1)

tests/unittest/test_s3_output.py

Walkthrough

Changes

The S3 pytest integration now transforms captured report sections, inlines or spools output, supports synchronous and deferred uploads, and drains orphaned spool files. Jenkins and integration runners enable native FD capture and perform post-test cleanup. Tests cover upload, recovery, xdist, capture ordering, and command wiring.

S3 deferred output workflow

Layer / File(s)	Summary
Report transformation and upload plugin `tests/test_common/s3_output.py`	`UploadLogPlugin` transforms pytest report sections, inlines small output, spools larger output, uploads synchronously or asynchronously, and exposes revised registration and CLI options.
Pending spool recovery `tests/test_common/s3_output.py`	`drain_pending_uploads` discovers orphaned spool files, validates ownership, uploads them concurrently, and removes completed spool state.
Pytest and Jenkins execution wiring `tests/test_common/s3_output_hooks.py`, `tests/integration/defs/test_unittests.py`, `jenkins/L0_Test.groovy`, `jenkins/scripts/slurm_run.sh`	Pytest workers register the plugin, commands use `--capture=fd` with deferred S3 uploads, and runners drain pending uploads after execution.
Upload behavior validation `tests/unittest/test_s3_output.py`, `tests/unittest/tools/test_test_to_stage_mapping.py`	Tests validate report transformation, upload modes, spool recovery, xdist registration, native capture ordering, timeout output, and Jenkins command construction.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Jenkins
  participant Pytest
  participant UploadLogPlugin
  participant Spool
  participant S3
  Jenkins->>Pytest: enable capture=fd and deferred S3 upload
  Pytest->>UploadLogPlugin: process pytest report sections
  UploadLogPlugin->>Spool: write oversized captured output
  UploadLogPlugin->>S3: upload or schedule deferred upload
  Jenkins->>Spool: drain pending uploads after pytest
  Spool->>S3: upload orphaned files

Suggested reviewers: qijune, schetlur-nv, zhanruisunch, tburt-nv

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description only contains the template and checkbox, with no actual issue summary or test coverage.	Add a short Description and Test Coverage section describing the problem, the solution, and the tests that verify it.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title matches the main change: switching S3 log handling to native pytest capture.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

tensorrt-cicd · 2026-07-22T14:14:40Z

PR_Github #60995 [ run ] triggered by Bot. Commit: 19e1af6 Link to invocation

tensorrt-cicd · 2026-07-22T15:14:03Z

PR_Github #60995 [ run ] completed with state FAILURE. Commit: 19e1af6
/LLM/main/L0_MergeRequest_PR pipeline #49254 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

Please check the failed tests and fix your PR
If you cannot view the failures, ask the CI triggerer to share details
Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

tensorrt-cicd · 2026-07-22T15:36:59Z

PR_Github #61007 [ run ] triggered by Bot. Commit: e9e5dd1 Link to invocation

tensorrt-cicd · 2026-07-22T20:08:00Z

PR_Github #61007 [ run ] completed with state FAILURE. Commit: e9e5dd1
/LLM/main/L0_MergeRequest_PR pipeline #49265 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

Please check the failed tests and fix your PR
If you cannot view the failures, ask the CI triggerer to share details
Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

Signed-off-by: Yiteng Niu <6831097+niukuo@users.noreply.github.com>

tensorrt-cicd · 2026-07-23T03:31:56Z

PR_Github #61180 [ run ] triggered by Bot. Commit: ba8cca9 Link to invocation

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_common/s3_output.py`:
- Around line 268-274: Update the hashlib.md5 call in normalize_test_name to
pass usedforsecurity=False, preserving the existing nodeid-derived suffix and
filename normalization behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 67cdefca-2930-4fea-beb8-6027f9e62363

📥 Commits

Reviewing files that changed from the base of the PR and between e9e5dd1 and ba8cca9.

📒 Files selected for processing (7)

jenkins/L0_Test.groovy
jenkins/scripts/slurm_run.sh
tests/integration/defs/test_unittests.py
tests/test_common/s3_output.py
tests/test_common/s3_output_hooks.py
tests/unittest/test_s3_output.py
tests/unittest/tools/test_test_to_stage_mapping.py

🚧 Files skipped from review as they are similar to previous changes (3)

tests/integration/defs/test_unittests.py
jenkins/L0_Test.groovy
tests/test_common/s3_output_hooks.py

tensorrt-cicd · 2026-07-23T08:22:06Z

PR_Github #61248 [ run ] triggered by Bot. Commit: ba8cca9 Link to invocation

tensorrt-cicd · 2026-07-23T08:25:07Z

PR_Github #61180 [ run ] completed with state ABORTED. Commit: ba8cca9

Link to invocation

tensorrt-cicd · 2026-07-23T16:14:28Z

PR_Github #61248 [ run ] completed with state FAILURE. Commit: ba8cca9
/LLM/main/L0_MergeRequest_PR pipeline #49486 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

Please check the failed tests and fix your PR
If you cannot view the failures, ask the CI triggerer to share details
Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

Signed-off-by: Yiteng Niu <6831097+niukuo@users.noreply.github.com>

tensorrt-cicd · 2026-07-23T16:50:43Z

PR_Github #61338 [ run ] triggered by Bot. Commit: e764c3c Link to invocation

tensorrt-cicd · 2026-07-23T20:16:56Z

PR_Github #61338 [ run ] completed with state FAILURE. Commit: e764c3c
/LLM/main/L0_MergeRequest_PR pipeline #49565 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

Please check the failed tests and fix your PR
If you cannot view the failures, ask the CI triggerer to share details
Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

tensorrt-cicd · 2026-07-24T01:19:08Z

PR_Github #61449 [ run ] triggered by Bot. Commit: e764c3c Link to invocation

tensorrt-cicd · 2026-07-24T03:20:43Z

PR_Github #61449 [ run ] completed with state SUCCESS. Commit: e764c3c
/LLM/main/L0_MergeRequest_PR pipeline #49671 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

Please check the failed tests and fix your PR
If you cannot view the failures, ask the CI triggerer to share details
Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

tensorrt-cicd · 2026-07-24T04:11:31Z

PR_Github #61495 [ run ] triggered by Bot. Commit: e764c3c Link to invocation

tensorrt-cicd · 2026-07-24T07:28:44Z

PR_Github #61495 [ run ] completed with state SUCCESS. Commit: e764c3c
/LLM/main/L0_MergeRequest_PR pipeline #49714 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

Please check the failed tests and fix your PR
If you cannot view the failures, ask the CI triggerer to share details
Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

tensorrt-cicd · 2026-07-24T08:13:44Z

PR_Github #61540 [ run ] triggered by Bot. Commit: e764c3c Link to invocation

tensorrt-cicd · 2026-07-24T08:59:38Z

PR_Github #61540 [ run ] completed with state SUCCESS. Commit: e764c3c
/LLM/main/L0_MergeRequest_PR pipeline #49751 completed with status: 'SUCCESS'

CI Report

Link to invocation

niukuo requested review from a team as code owners July 22, 2026 14:05

niukuo requested review from QiJune, ZhanruiSunCh, ruodil, schetlur-nv and tburt-nv July 22, 2026 14:05

github-actions Bot assigned niukuo Jul 22, 2026

niukuo force-pushed the test_output branch from 19e1af6 to e9e5dd1 Compare July 22, 2026 15:35

ruodil approved these changes Jul 23, 2026

View reviewed changes

niukuo added 2 commits July 23, 2026 11:30

[TRTLLMINF-191][infra] Use native pytest capture for S3 logs

53dd0e8

Signed-off-by: Yiteng Niu <6831097+niukuo@users.noreply.github.com>

[TRTLLMINF-191][infra] Deduplicate S3 capture uploads across reports

ba8cca9

Signed-off-by: Yiteng Niu <6831097+niukuo@users.noreply.github.com>

niukuo force-pushed the test_output branch from e9e5dd1 to ba8cca9 Compare July 23, 2026 03:30

coderabbitai Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread tests/test_common/s3_output.py

[TRTLLMINF-191][infra] Merge pytest capture streams before upload

e764c3c

Signed-off-by: Yiteng Niu <6831097+niukuo@users.noreply.github.com>

yiqingy0 approved these changes Jul 24, 2026

View reviewed changes

Uh oh!

Conversation

niukuo commented Jul 22, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dev Engineer Review

QA Engineer Review

tests/unittest/test_s3_output.py (rewritten/expanded behavioral coverage)

tests/unittest/tools/test_test_to_stage_mapping.py

tests/integration/defs/test_unittests.py

Description

Test Coverage

PR Checklist

GitHub Bot Help

Uh oh!

niukuo commented Jul 22, 2026

Uh oh!

coderabbitai Bot commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

tensorrt-cicd commented Jul 22, 2026

Uh oh!

tensorrt-cicd commented Jul 22, 2026

Uh oh!

tensorrt-cicd commented Jul 22, 2026

Uh oh!

tensorrt-cicd commented Jul 22, 2026

Uh oh!

tensorrt-cicd commented Jul 23, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tensorrt-cicd commented Jul 23, 2026

Uh oh!

tensorrt-cicd commented Jul 23, 2026

Uh oh!

tensorrt-cicd commented Jul 23, 2026

Uh oh!

tensorrt-cicd commented Jul 23, 2026

Uh oh!

tensorrt-cicd commented Jul 23, 2026

Uh oh!

tensorrt-cicd commented Jul 24, 2026

Uh oh!

tensorrt-cicd commented Jul 24, 2026

Uh oh!

tensorrt-cicd commented Jul 24, 2026

Uh oh!

tensorrt-cicd commented Jul 24, 2026

Uh oh!

tensorrt-cicd commented Jul 24, 2026

Uh oh!

tensorrt-cicd commented Jul 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

niukuo commented Jul 22, 2026 •

edited by coderabbitai Bot

Loading

`tests/unittest/test_s3_output.py` (rewritten/expanded behavioral coverage)

`tests/unittest/tools/test_test_to_stage_mapping.py`

`tests/integration/defs/test_unittests.py`

coderabbitai Bot commented Jul 22, 2026 •

edited

Loading